Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add the backups-create command #452

Merged
merged 8 commits into from
Jul 22, 2019
Merged

Add the backups-create command #452

merged 8 commits into from
Jul 22, 2019

Conversation

EtienneM
Copy link
Member

@EtienneM EtienneM commented Jul 17, 2019

Wait for Scalingo/go-scalingo#116 to be merged merged!
Wait for #449 to be merged merged!
Wait for Scalingo/go-scalingo#115 to be merged merged!

╰─$ scalingo --app my-app --addon ad-78fbfa75-367d-43da-98a9-9d40d3ea0dac backups-create
-----> Successfully ordered to make a backup

Fix #450

@EtienneM EtienneM marked this pull request as ready for review July 17, 2019 15:16
@EtienneM EtienneM requested a review from johnsudaar July 17, 2019 15:17
return err
}

io.Status("Successfully ordered to make a backup")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice if we could follow the backup progression.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well actually it was not really well designed. The backend should have returned 202 and an operation URL in the Location header. Then we could have follow the process... ^^

That being said I can add a backups-show command and update the status message to state:

Successfully scheduled a new backup. Type "scalingo backups-show backup-id" to follow the progress

If it's what you want, I would prefer to do it in a separate PR. OK?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

	for !backup.IsFinished() {
		spinner.Lock()
		if backup.Status == types.BackupStatusScheduled {
			spinner.Suffix = " Waiting for the backup to start"
		} else {
			spinner.Suffix = " Waiting for the backup to finish"
		}
		spinner.Unlock()

		time.Sleep(1 * time.Second)

		backup, err = client.BackupGet(dbId, backup.ID.Hex())
		if err != nil {
			return errors.Wrap(err, "Fail to refresh backup state")
		}
	}

This is what i did for the db-cli. I think that this might be enough no ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean BackupGet or GetBackup? 😆

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But yes, good idea!

@EtienneM
Copy link
Member Author

╰─$ scalingo --app my-app --addon ad-78fbfa75-367d-43da-98a9-9d40d3ea0dac backups-create
⣽ Waiting for the backup to finish 

What a good idea 😻

@EtienneM EtienneM requested a review from johnsudaar July 22, 2019 09:31
@jonathanperret
Copy link
Contributor

@johnsudaar @EtienneM FYI this broke one of our scripts that was using the backup-download command. I looks like here (https://github.com/Scalingo/cli/pull/452/files#diff-6eae5f17654b235c68ab2bb17d69c9f2L32) you tried to keep the old backup-download command functional but with a warning; but it looks like the legacy command descriptor is missing the Flags parameter causing it to reject every invocation that has any flag, and not show the warning:

$ scalingo -a my-app backup-download --addon st-12345-67890 --backup abcdef12345
Incorrect Usage: flag provided but not defined: -addon

NAME:
   scalingo backup-download - Download a backup

USAGE:
   scalingo backup-download [command options] [arguments...]

CATEGORY:
   Addons

DESCRIPTION:
     Download a specific backup:
    $ scalingo --app my-app --addon addon_uuid backups-download --backup my_backup

    # See also 'backups' and 'addons'

OPTIONS:
   --region value  Name of the region to use

Fail to run command: flag provided but not defined: -addon

@EtienneM
Copy link
Member Author

@jonathanperret The issue has been fixed an a new bugfix release (v1.15.1) has been released. Sorry for the inconvenience.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a backups-create command
3 participants